Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DM-46287: Make image binning a subtask for IsrTask #349

Merged
merged 4 commits into from
Sep 26, 2024
Merged

Conversation

jrmullaney
Copy link
Contributor

No description provided.

Copy link
Contributor

@czwa czwa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few suggested changes, but I'm happy here. It would be good to have the pipe_tasks task call this code as well, but that can be on a follow-up ticket (I'll file that now).

storageClass=self.binnedExposure.storageClass,
dimensions=frozenset(config.exposureDimensions),
)
if config and config.exposureStorageClass != self.inputExposure.storageClass:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you have any expected cases where the storageClass would need to change?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this also would cause problems with the check on L193, as that will raise if the storageClass isn't an Exposure. This does allow ExposureD, so maybe a comment here mentioning that although the storageClass can be changed, if it isn't some kind of Exposure, it's going to cause problems.

Copy link
Contributor Author

@jrmullaney jrmullaney Sep 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added this to ensure that the output storage class would always match the input storage class. While I could have the input and output storage classes as the generic Exposure, and it would be happy with any of the sub-types as input, any subtype information (I,F,D etc) would be lost on the output (at least that's my understanding).

I can certainly add the comment. [Late edit: Rather than add a comment, I've added an extra line to the config doc to state that it must be of type Exposure or one of its sub-types.]

python/lsst/ip/isr/binExposureTask.py Outdated Show resolved Hide resolved
python/lsst/ip/isr/binExposureTask.py Show resolved Hide resolved
python/lsst/ip/isr/isrTask.py Show resolved Hide resolved
python/lsst/ip/isr/isrTask.py Show resolved Hide resolved
tests/test_binExposure.py Show resolved Hide resolved
@jrmullaney jrmullaney force-pushed the tickets/DM-46287 branch 2 times, most recently from 5851362 to 6c17775 Compare September 25, 2024 17:30
IsrTask subtasks BinExposureTask to perform
exposure binning.
IsrTaskLSST subtasks BinExposureTask to perform
exposure binning.
@jrmullaney jrmullaney merged commit 497283e into main Sep 26, 2024
2 checks passed
@jrmullaney jrmullaney deleted the tickets/DM-46287 branch September 26, 2024 19:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants